Skip to content

ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts#11944

Closed
jonkeane wants to merge 3 commits into
apache:masterfrom
jonkeane:ARROW-14510-clean-build
Closed

ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts#11944
jonkeane wants to merge 3 commits into
apache:masterfrom
jonkeane:ARROW-14510-clean-build

Conversation

@jonkeane

Copy link
Copy Markdown
Member

No description provided.

@github-actions

Copy link
Copy Markdown

@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions

Copy link
Copy Markdown

Revision: f37cc99

Submitted crossbow builds: ursacomputing/crossbow @ actions-1291

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions

Copy link
Copy Markdown

Revision: 450f251

Submitted crossbow builds: ursacomputing/crossbow @ actions-1292

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-ubuntu-18.04-r-sanitizer test-fedora-r-clang-sanitizer test-r-linux-valgrind test-ubuntu-default-docs

@github-actions

Copy link
Copy Markdown

Revision: 01b8dc7

Submitted crossbow builds: ursacomputing/crossbow @ actions-1293

Task Status
test-fedora-r-clang-sanitizer Azure
test-r-linux-valgrind Azure
test-ubuntu-18.04-r-sanitizer Azure
test-ubuntu-default-docs Azure

@jonkeane jonkeane changed the title ARROW-14510: [R] run make clean when building docs ARROW-14510: [R] ensure that docker runs don't use host-built artifacts Dec 14, 2021
@jonkeane jonkeane changed the title ARROW-14510: [R] ensure that docker runs don't use host-built artifacts ARROW-14510: [R] [CI] ensure that docker runs don't use host-built artifacts Dec 14, 2021
@jonkeane

Copy link
Copy Markdown
Member Author

The two sanitizer failures are legit (and not caused by this ticket — they look like they are from #11850)

@thisisnic thisisnic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise LGTM

Comment thread ci/scripts/r_valgrind.sh

popd

popd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo or is it needed again here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are popds even needed here? My assumption is shell scripts don't influence the state after completing.

@jonkeane jonkeane Dec 14, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added another pushd up above, so I added this down here to match the flow — but you're right that this shouldn't change the state or anything so is probably not needed.

@rok rok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Out of curiosity - wouldn't ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} as suggested here also solve this problem? (Sorry for my ignorance if this is a completely off mark.)

Comment thread ci/scripts/r_valgrind.sh

popd

popd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are popds even needed here? My assumption is shell scripts don't influence the state after completing.

@jonkeane

Copy link
Copy Markdown
Member Author

Re: ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} That should solve the problem (though reading WRE it was't 100% clear if the ./cleanup file needed to be up to date with all the files that needed to be removed (in which case it could still fail if it's not up to date for some reason) or if --clean would do that on it's own). But more importantly: that would remove the built artifacts from the directory which might force a re-compile elsewhere. This fix intentionally tried to not impact the shared directory between docker and the host so that if the developer relies on keeping the artifacts around between builds, those will not be overwritten or deleted.

@rok

rok commented Dec 14, 2021

Copy link
Copy Markdown
Member

Re: ${R_BIN} CMD INSTALL --clean ${INSTALL_ARGS} That should solve the problem (though reading WRE it was't 100% clear if the ./cleanup file needed to be up to date with all the files that needed to be removed (in which case it could still fail if it's not up to date for some reason) or if --clean would do that on it's own). But more importantly: that would remove the built artifacts from the directory which might force a re-compile elsewhere. This fix intentionally tried to not impact the shared directory between docker and the host so that if the developer relies on keeping the artifacts around between builds, those will not be overwritten or deleted.

Got it! That makes sense, thanks for the explanation :).

@jonkeane jonkeane closed this in acce836 Dec 17, 2021
@ursabot

ursabot commented Dec 17, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7cf7442 and contender = acce836. acce836 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] ursa-i9-9960x
[Finished ⬇️0.31% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants